feat: add tracking log for completion events #245
Conversation
|
Thanks for the pull request, @Ian2012! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
Ian2012
force-pushed
the
cag/add-event-tracking
branch
2 times, most recently
from
3fa245a
to
27cd650
Compare
Ian2012
changed the title
Ian2012
force-pushed
the
cag/add-event-tracking
branch
from
27cd650
to
2a8e44c
Compare
completion/models.py
Outdated
| @@ -126,6 +131,20 @@ def submit_completion(self, user, block_key, completion): | |||
| "BlockCompletion.objects.submit_completion should not be \ | |||
| called when the feature is disabled." | |||
| ) | |||
|
|
|||
| tracker.emit( | |||
| str(BLOCK_COMPLETION_CHANGED_EVENT_TYPE), | |||
There was a problem hiding this comment.
Why is there a str here? I think based on our slack conversation this should get moved into its own method and accept the model instance?
|
Pinged the last couple of people who seem to have done work in this repo, lmk if there are better reviewers. 😄 |
Ian2012
force-pushed
the
cag/add-event-tracking
branch
from
4deff25
to
ed45d2d
Compare
| @@ -41,7 +41,7 @@ def setUp(self): | |||
| self.set_up_completion() | |||
|
|
|||
| def test_changed_value(self): | |||
| with self.assertNumQueries(6): # Get, update, 2 * savepoints, 2 * exists checks | |||
| with self.assertNumQueries(7): # 2 * Get, update, 2 * savepoints, 2 * exists checks | |||
There was a problem hiding this comment.
These are a little concerning, do you know where the extra queries are coming from?
There was a problem hiding this comment.
There is one additional for the obj.emit_tracking_log() method.
|
Hey @mphilbrick211! Is there any reviewer for this PR? This tracking log is really important for aspects |
|
Hi @openedx/masters-devs-cosmonauts! Would someone please be able to get this reviewed/merged for us? Thanks! |
mphilbrick211
added
the
waiting for eng review
|
@mphilbrick211 adding a ticket to our backlog to handle the merge and release for this |
Ian2012
force-pushed
the
cag/add-event-tracking
branch
3 times, most recently
from
76d942b
to
91b2087
Compare
|
@zacharis278 Is there anything else we need to do before merging this PR? |
|
Hi @zacharis278! Is there an ETA on this? I know you mentioned it was being added to your backlog. Checking in because it's directly blocking this PR which is important to the Aspects work. Please let us know if there's any updates. Thanks! |
Ian2012
force-pushed
the
cag/add-event-tracking
branch
from
91b2087
to
82551e6
Compare
|
cc @UsamaSadiq @zubairshakoorarbisoft Can you take a look at this PR? This is really important event for the Aspects project |
|
Looks good to me! |
|
@Ian2012 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
Hi @UsamaSadiq thanks for merging, can someone roll a release as well? Looks like there hasn't been one in a while. |
Description: This PR emits a tracking log for every completion event (created/updated).
Related issue: openedx/openedx-aspects#88
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
edx.completion.block_completion.changedis emitted with the proper informationReviewers:
Merge checklist:
Post merge:
finished.